Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor error returning timing of doSearch function #1996

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Apr 4, 2023

Description:

LB gateway should use early return error logic for performance, I changed it.
And errors.Join usually uses 1, or 2 errors it should be optimize, I did it.
internal/net/grpc/client span didn't record its error corectly, I fixed it.

Related Issue:

Versions:

  • Go Version: 1.20.2
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 2.0.9

Checklist:

Special notes for your reviewer:

@kpango kpango changed the title refactor error returning timing of doSearch function [Proposal] refactor error returning timing of doSearch function Apr 4, 2023
@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Apr 4, 2023

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.20 ⚠️

Comparison is base (48e14c1) 29.63% compared to head (4203241) 29.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1996      +/-   ##
==========================================
- Coverage   29.63%   29.44%   -0.20%     
==========================================
  Files         365      365              
  Lines       34245    34429     +184     
==========================================
- Hits        10150    10139      -11     
- Misses      23676    23871     +195     
  Partials      419      419              
Impacted Files Coverage Δ
internal/errors/errors.go 53.36% <0.00%> (-27.74%) ⬇️
internal/net/grpc/client.go 0.00% <0.00%> (ø)
pkg/gateway/lb/handler/grpc/handler.go 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kpango kpango force-pushed the refactor/gateway-lb/search-error-return-timing branch from 7c96933 to cd0e69f Compare April 5, 2023 05:46
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 5, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4203241
Status: ✅  Deploy successful!
Preview URL: https://705f897b.vald.pages.dev
Branch Preview URL: https://refactor-gateway-lb-search-e.vald.pages.dev

View logs

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Apr 5, 2023

[GEN TEST] Failed to generate tests.

@kpango kpango force-pushed the refactor/gateway-lb/search-error-return-timing branch from cd0e69f to 3cc3cb6 Compare April 5, 2023 05:59
@github-actions github-actions bot added size/XXL and removed size/XL labels Apr 5, 2023
@kpango kpango force-pushed the refactor/gateway-lb/search-error-return-timing branch 4 times, most recently from c25fbce to 559256e Compare April 5, 2023 07:52
Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango force-pushed the refactor/gateway-lb/search-error-return-timing branch from 559256e to 4203241 Compare April 5, 2023 08:06
@kpango kpango changed the title [Proposal] refactor error returning timing of doSearch function Refactor error returning timing of doSearch function Apr 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Apr 5, 2023

Profile Report

typevald-agent-ngtvald-lb-gatewayvald-discoverervald-manager-index
cpu
heap
other images

switch {
case errs[0] != nil && errs[1] != nil && !Is(errs[0], errs[1]):
var es []error
switch x := errs[1].(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [golangci] reported by reviewdog 🐶
type switch on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)

case 1:
return e.errs[0].Error()
}
b := make([]byte, 0, len(e.errs)*16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 16, in detected (gomnd)

if errs[0] != nil {
return errs[0]
}
case 2:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 2, in detected (gomnd)

@kpango kpango merged commit 86cc8c8 into main Apr 7, 2023
@kpango kpango deleted the refactor/gateway-lb/search-error-return-timing branch April 7, 2023 04:02
@ykadowak ykadowak mentioned this pull request Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants